feat: `ngen_cal_model_configure` model hook #161

aaraney · 2024-08-09T20:43:15Z

Add model hook: ngen_cal_model_configure hook that mirrors the existing ngen_cal_configure hook.

    @hookspec
    def ngen_cal_model_configure(self, config: ngen.cal.model.ModelExec) -> None:
        """
        Called before calibration begins.
        This allow plugins to perform initial configuration.

        Plugins' configuration data should be provided using the
        `plugins_settings` field in the `model` section of an `ngen.cal`
        configuration file.
        By convention, the name of the plugin should be used as top level key in
        the `plugin_settings` dictionary.
        """

Additions

ngen_cal_model_configure model hook; Called before calibration begins. This allow plugins to perform initial configuration.
unwrap method added to ngen.cal.ngen.Ngen. Returns the instance's __root__ attribute.

Changes

Agent now takes a Model instance as a parameter. Previously, the raw config was passed.

hellkite500

The change in behavior of the Agent and its interaction with the model config is quite likely to break.

python/ngen_cal/src/ngen/cal/agent.py

hellkite500 · 2024-08-14T15:46:39Z

python/ngen_cal/src/ngen/cal/agent.py


        if self._job is None:
-            self._job = JobMeta(model_conf['type'], workdir, log=log)


These steps are minutely important to the use of the Agent, and I don't think they can be dropped...The agent manipulates the configs to ensure root/workdirs are set appropriately under the automatically created agent working dirs.

I don't think they are dropped? I was just pulling them from the pydantic model instance now instead of the dictionary. Probably missing something thought.

Sorry, this comment ended up being unclear, it was intended for lines 81-83 where workdir and binary are configured relative to the Agent's dynamically created workdir. These are critical for correct operation of agents especially in PSO where multiple agents are running simultaneously.

As noted in disscussion with @aaraney the current resolved_binary isn't actually working as intended, and is its own bug which can be resolved in this context.

Opened #167 to track this.

hellkite500 · 2024-08-15T18:03:04Z

python/ngen_cal/src/ngen/cal/agent.py

-        resolved_binary = Path(model_conf['binary']).resolve()
-        model_conf['workdir'] = self.job.workdir
-        self._model = Model(model=model_conf, binary=resolved_binary)
+            self._job = JobMeta(ngen_model.type, workdir, log=log)


I think we are still missing the updated model workdir. An agent has a job which nests information and job/model files under a dynamically created workdir job.workdir. Any model instance associated with that job must have its workdir aligned with the job.workdir, so we need to update the model reference like such

Suggested change

self._job = JobMeta(ngen_model.type, workdir, log=log)

self._job = JobMeta(ngen_model.type, workdir, log=log)

ngen_model.workdir = self.job.workdir

hellkite500 · 2024-08-15T18:07:50Z

python/ngen_cal/src/ngen/cal/__main__.py

    plugins = cast(List[Union[Callable, ModuleType]], general.plugins)
    plugin_manager = setup_plugin_manager(plugins)

    print(_loaded_plugins(plugin_manager))

    # setup plugins
    plugin_manager.hook.ngen_cal_configure(config=general)
+    model_inner._plugin_manager.hook.ngen_cal_model_configure(config=model_inner)


An agent effectively side effects the Model it associates with, particularly setting the workdir for that model run. Because of this, we probably need to initialize the Agent before passing the Model to anything else, including plugins.

Suggested change

model_inner._plugin_manager.hook.ngen_cal_model_configure(config=model_inner)

# Initialize the starting agent

# WARNING: Agent sets the model workdir, so initialize it before using the model object for anything that may require an accurate workdir

agent = Agent(model, general.workdir, general.log, general.restart, general.strategy.parameters)

model_inner._plugin_manager.hook.ngen_cal_model_configure(config=model_inner)

hellkite500 · 2024-08-15T18:08:27Z

python/ngen_cal/src/ngen/cal/__main__.py

+
+    # Initialize the starting agent
+    agent = Agent(model, general.workdir, general.log, general.restart, general.strategy.parameters)


Suggested change

# Initialize the starting agent

agent = Agent(model, general.workdir, general.log, general.restart, general.strategy.parameters)

Agent mutates the model config; If ngen_cal_model_configure is called before the Agent is initialized, it won't get the final view of the config before calibration. Co-authored-by: Nels <[email protected]>

Co-authored-by: Nels <[email protected]>

aaraney added the ngen.cal Related to ngen.cal package label Aug 9, 2024

aaraney requested a review from hellkite500 August 9, 2024 20:43

aaraney self-assigned this Aug 9, 2024

aaraney changed the title ~~Add model config plugin~~ feat: ngen_cal_model_configure model hook Aug 9, 2024

aaraney marked this pull request as draft August 9, 2024 20:55

aaraney marked this pull request as ready for review August 9, 2024 21:00

aaraney mentioned this pull request Aug 12, 2024

ngen.cal support for t-route's netcdf, parquet, and 3 csv output formats variants #162

Merged

hellkite500 requested changes Aug 14, 2024

View reviewed changes

aaraney force-pushed the add-model-config-plugin branch from d6e4ca1 to 1f9770f Compare August 15, 2024 17:45

hellkite500 reviewed Aug 15, 2024

View reviewed changes

aaraney force-pushed the add-model-config-plugin branch from db0819f to 6a3ca77 Compare August 15, 2024 18:23

aaraney added 9 commits August 15, 2024 14:33

feat: add ngen_cal_model_configure hook

2cae8a5

test: ngen_cal_model_configure

17badd5

refactor: Agent dep injects a Model instead of taking a raw config

5e3ad14

feat: Ngen.unwrap convenience method. returns __root__

c101bf0

refactor: construct Model and inject into Agent

b1a2cea

feat: call ngen_cal_model_configure plugin

56e96db

refactor(test): inject Model instance

88f5ae3

chore: update comments and improve naming

29c3528

style: type hint

dd18e0e

aaraney force-pushed the add-model-config-plugin branch from 6a3ca77 to dd18e0e Compare August 15, 2024 18:34

aaraney and others added 2 commits August 15, 2024 14:38

fix(pr-review): call ngen_cal_model_configure after Agent init

813def9

Agent mutates the model config; If ngen_cal_model_configure is called before the Agent is initialized, it won't get the final view of the config before calibration. Co-authored-by: Nels <[email protected]>

fix(pr-review): sync model and job meta workdirs

94cd2aa

Co-authored-by: Nels <[email protected]>

hellkite500 approved these changes Aug 15, 2024

View reviewed changes

hellkite500 merged commit aab8cf8 into NOAA-OWP:master Aug 15, 2024
12 checks passed

aaraney deleted the add-model-config-plugin branch August 15, 2024 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `ngen_cal_model_configure` model hook #161

feat: `ngen_cal_model_configure` model hook #161

aaraney commented Aug 9, 2024 •

edited

Loading

hellkite500 left a comment

hellkite500 Aug 14, 2024

aaraney Aug 14, 2024 •

edited

Loading

hellkite500 Aug 15, 2024

aaraney Aug 15, 2024

hellkite500 Aug 15, 2024

hellkite500 Aug 15, 2024

hellkite500 Aug 15, 2024


		if self._job is None:
		self._job = JobMeta(model_conf['type'], workdir, log=log)

	self._job = JobMeta(ngen_model.type, workdir, log=log)
	self._job = JobMeta(ngen_model.type, workdir, log=log)
	ngen_model.workdir = self.job.workdir


		# Initialize the starting agent
		agent = Agent(model, general.workdir, general.log, general.restart, general.strategy.parameters)

feat: ngen_cal_model_configure model hook #161

feat: ngen_cal_model_configure model hook #161

Conversation

aaraney commented Aug 9, 2024 • edited Loading

Additions

Changes

hellkite500 left a comment

Choose a reason for hiding this comment

hellkite500 Aug 14, 2024

Choose a reason for hiding this comment

aaraney Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

hellkite500 Aug 15, 2024

Choose a reason for hiding this comment

aaraney Aug 15, 2024

Choose a reason for hiding this comment

hellkite500 Aug 15, 2024

Choose a reason for hiding this comment

hellkite500 Aug 15, 2024

Choose a reason for hiding this comment

hellkite500 Aug 15, 2024

Choose a reason for hiding this comment

feat: `ngen_cal_model_configure` model hook #161

feat: `ngen_cal_model_configure` model hook #161

aaraney commented Aug 9, 2024 •

edited

Loading

aaraney Aug 14, 2024 •

edited

Loading